Test background_fetch rule matching - #13563

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Reviewed the new test against the plugin's actual rule evaluation. The wildcard case is solid, but two of the three scenarios don't verify what they're meant to — including the positive control, which currently can't observe a background fetch at all.

1. The positive control never exercises the background fetch path

MakeOriginServer defaults to lookup_key='{PATH}' (microserver.test.ext:31), and microserver stores entries with a plain dict assignment:

glb.replayDict[key] = response_header      # microserver.py:156

_configure_server() registers two responses for path /allowed — the 206 from the _add_range_response loop, then the explicit 200 — so the second overwrites the first. Every request to /allowed gets the 200 OK with the full 10-byte body, including the client's Range: bytes=0-4 request.

That matters because background_fetch only engages on a partial response:

// background_fetch.cc:536
if (TS_HTTP_STATUS_PARTIAL_CONTENT == status || (config->allow304() && TS_HTTP_STATUS_NOT_MODIFIED == status)) {

A 200 means no background fetch is ever started, so the "unrestricted path" demonstrates nothing about the plugin, and the 200 OK assertion passes for the wrong reason — it's asserting the origin ignored the Range header.

Two changes needed:

• Key the origin so the range and non-range entries for /allowed can coexist. microserver supports {PATH}, {HOST}, {URL} and {%Field} (servers.py:139-160), so Test.MakeOriginServer("server", lookup_key='{PATH}{%Range}') works, and the allowed run then expects 206 Partial Content like the others.

• Assert the positive signal. There's a debug line for exactly this:

  // background_fetch.cc:393
  Dbg(Bg_dbg_ctl, "Starting background fetch, replaying:");

  Without it, nothing in the test distinguishes "background fetch ran" from "the plugin did nothing," which is the whole point of having a control.

2. The Content-Length rule has no discriminating assertion

Both exclusion runs emit the same generic line:

// configs.cc:209
Dbg(Bg_dbg_ctl, "found %s rule match", r._exclude ? "exclude" : "include");

So Testers.ContainsExpression(r"found exclude rule match") is satisfied by the wildcard case on its own. If the Content-Length threshold stopped matching — a parse regression, an operator change, a comparison flip — this test still passes. The 206 Partial Content status assertion doesn't help either: the client gets a 206 whether or not the background fetch was suppressed.

The wildcard rule is uniquely pinned, via "Found X-Skip-Bg wild card" — that half is well done. The Content-Length rule, which is first in the PR title and summary, is the untested one.

Cheapest improvement is to also assert the parse line, which at least catches a config-syntax regression:

// configs.cc:174
"adding background_fetch content length rule {} for {}: {}"

A stronger version would add a fourth remap with exclude Content-Length >1000 — which should not match a 5-byte response — and assert a background fetch does start for it. That gives you a matched pair around the threshold instead of a single one-sided check.

Also worth a comment in the test: <1000 parses to LESS_THAN_OR_EQUAL (configs.cc:151), not strict less-than. The config syntax reads like <, so the boundary behavior is surprising, and a test is the natural place to record it.

3. StillRunningAfter assigned twice drops the first check

tr.StillRunningAfter = self._ts
tr.StillRunningAfter = self._server

TesterSet.Assign replaces the whole list:

def Assign(self, value):
    self._testers = [self._create_tester(value)]   # testerset.py:47
def Add(self, value):
    self._testers += [self._create_tester(value)]

= routes to Assign, += to Add. So only _server is verified still running after each run — if traffic_server crashes mid-test, the run passes. Same in _wait_for_wildcard_exclusion().

This is a pre-existing pattern in the tree (160 .test.py files do it), so not something to hold this PR on, and you're the one converting all of it to pytest where the read-only expectation objects make it impossible. But since this file is new, += on the second line costs nothing.

4. The waiter waits on the earlier of the two lines

_wait_for_wildcard_exclusion() blocks on "Found X-Skip-Bg wild card", which is logged in check_value (rules.cc:118) and returns true, after which check_field_configured's caller logs "found exclude rule match" (configs.cc:209). So the line the waiter confirms is flushed is emitted before the other asserted line. The narrow flush race the waiter exists to close is still open for "found exclude rule match".

Waiting on "found exclude rule match" instead covers both, since it's the later of the two.

Smaller items

• Test.SkipUnless(Condition.PluginExists('background_fetch.so'),) has a stray trailing comma inside the call.
• The tester description "wildcard request header rule should match by value" is slightly off — * matches presence, any value; check_value short-circuits before reading the value at all. "should match on presence regardless of value" is closer.
• _add_curl_run derives the path from the host with host.split(".")[0], which is neat but means the path and hostname can't be varied independently. A path parameter would read more plainly, especially if the >1000 case above gets added.
• The 206 responses pair Content-Range: bytes 0-4/10 with Content-Length: 5 and a 5-byte body, which is consistent — worth keeping in mind that the /10 total is what makes a background fetch worthwhile, so it shouldn't drift from the 200 response's Content-Length: 10.

The structure itself is good — class-based, one config file per rule type, one remap per scenario, and per-host isolation so the rules can't interfere. It's the assertions that need to become specific enough to fail when the rules break.

🟢 Approval recommended

Verified the update against the plugin and microserver code — all four points are genuinely addressed, not just claimed.

1. Positive control now exercises the background fetch path. lookup_key="{PATH}{%Range}" makes getLookupKey (servers.py:132) produce Range:bytes=0-4/allowed vs /allowed, so the 206 and 200 entries coexist instead of the second overwriting the first. That second entry does double duty: the plugin strips Range/If-Range before replaying (background_fetch.cc:285), so the non-range response is exactly what the background fetch itself needs from the origin.

The marker assertion is a real signal. dump_headers has exactly one call site, inside the Bg_dbg_ctl.on() block at background_fetch.cc:394, so X-Background-Fetch-Test: <marker> can only reach traffic_out when a background fetch actually starts. That makes the ExcludesExpression checks for small and wildcard meaningful too, not just absence-of-evidence.

2. Content-Length now has a matched pair around the threshold. The added exclude Content-Length >1000 remap parses to GREATER_THAN_OR_EQUAL (configs.cc:154) and doesn't match the 5-byte response, so its marker must be present while small's must be absent. A parse regression, an operator change, or a comparison flip in either direction now fails. The parse-line regex matches the real format string at configs.cc:174 (the leading </> is dropped before printing, so Content-Length: 1000 is correct). Good call documenting the inclusive-operator behavior in the config section.

3. += on the second StillRunningAfter, in both places.

4. The waiter now blocks on the last run's marker, emitted after both exclusion runs, so it's a proper flush barrier for the earlier assertions rather than the earlier of the two lines.

Two nits, neither worth another round: the generic found exclude rule match assertion is now subsumed by the markers, and the parse-line regex can't distinguish the < config from the > one since both render Content-Length: 1000. The behavioral pair covers what matters.

LGTM.